Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug caused by support OSM traffic signal directions #6724

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

GitBenjamin
Copy link
Contributor

@GitBenjamin GitBenjamin commented Oct 23, 2023

I try fix a bug caused by support OSM traffic signal directions. With mjjbell's help, I learned a lot. I hope to get @mjjbell help and support.

Issue

#6153 (comment)

Tasklist

Requirements / Relations

#6153
#6339
#6153 (comment)

@GitBenjamin
Copy link
Contributor Author

Hi, @SiarheiFedartsou. I added CHANGELOG and removed the comment. Thanks!

@@ -327,6 +330,19 @@ void GraphCompressor::Compress(const std::unordered_set<NodeID> &barrier_nodes,
reverse_node_weight_penalty,
reverse_node_duration_penalty);

auto add_expanded_traffic_signal =
[&expanded_traffic_signals,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to traffic signals needs to be made in-place, as they're subsequently used as part of the edge-expanded graph generation too. As mentioned in the other thread, we could make a compressor wrapper for traffic signals, much like we do with TurnPaths.

void TurnPathCompressor::Compress(const NodeID from, const NodeID via, const NodeID to)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to traffic signals needs to be made in-place, as they're subsequently used as part of the edge-expanded graph generation too. As mentioned in the other thread, we could make a compressor wrapper for traffic signals, much like we do with TurnPaths.

void TurnPathCompressor::Compress(const NodeID from, const NodeID via, const NodeID to)

Thank you very much for your guidance 😄. I will try to make a compressor wrapper for traffic signals, like you do with TurnPaths. I need some time to complete the modification, and add test cases to the Cucumber suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mjjbell, I'm sorry that i only started working on this recently. I study the TurnPathCompressor class, but i find that the traffic signal problem seems to be different.

--- Related method ---

const auto &from_node =
isTrivial ? node_along_road_entering
: m_compressed_edge_container.GetLastEdgeSourceID(node_based_edge_from);

const auto is_traffic_light = m_traffic_signals.HasSignal(from_node, intersection_node);

The TrafficSignals info used during the generation of the edge-expanded graph. But the HasSignal method is not affected by src/extractor/graph_compressor.cpp. As the GetLastEdgeSourceID method returns the original node_id of the segment (even if the node has been compressed), the from_node & intersection_node are the original relation in the OSM nodes. So we don't have to worry about the HasSignal losing traffic signal feature due to compression.

Do we still need to make a compressor wrapper for traffic signals?

Copy link
Member

@mjjbell mjjbell Mar 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, well, this is currently working only because we're not considering the compression here.

If we start compressing the traffic signals, this becomes simpler, as we will no longer care if it's a trivial edge or not.

For example, this currently works as expected

@routing @car @traffic_light
Feature: Car - Handle traffic lights

    Background:
        Given the profile "car"

    Scenario: Car - Traffic signal direction turn with edge compression
        Given the node map
            """
                    d
                    |
                    2
                    |
            a-1-b - c--f
                    |
                    e

            """

        And the ways
            | nodes | highway |
            | abc   | primary |
            | ce    | primary |
            | cd    | primary |
            | ce    | primary |

        And the nodes
            | node | highway         | traffic_signals:direction | # |
            | c    | traffic_signals | forward                   | ambiguous, but happens in OSM

        When I route I should get
            | from | to | time   | weight | #                             |
            | 1    | 2  |  35.1s | 35.1   | turn with traffic light    |
            | 2    | 1  |  29.9s | 29.9   | turn with no traffic light |

But it would fail if we correctly compress the traffic signals and don't update the edge based graph factory logic.

The case that is currently not being handled properly is when there is no turn, as shown here
#6153 (comment)

Copy link
Contributor Author

@GitBenjamin GitBenjamin Mar 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, do you still need me to make a compressor wrapper for traffic signals ?😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I included it in the traffic signal, as it's simple enough.

void Compress(NodeID from, NodeID via, NodeID to)

@mjjbell
Copy link
Member

mjjbell commented Nov 1, 2023

It would be beneficial to add minimal test cases to the Cucumber suite that capture the two problems you have identified.

@GitBenjamin
Copy link
Contributor Author

GitBenjamin commented Jan 4, 2024

It would be beneficial to add minimal test cases to the Cucumber suite that capture the two problems you have identified.

Hi, @mjjbell. I added two test cases to the Cucumber suite, but it only capture one problems (Problem 1). Another problem may require a more complex routing network to catch.😔

@mjjbell
Copy link
Member

mjjbell commented Mar 16, 2024

@GitBenjamin I'm going to add the fix to this PR for problem 1.

It's unclear if problem 2 would also be fixed, but we can address that in a separate PR once we have identified a minimal reproduction test case.

Unidirectional traffic signal segments are currently not compressed.
This means traffic signals which are not on turns can be missed and
not applied the correct penalty.

This commit changes this behaviour to correctly handle the graph
compression. Additional tests are added to ensure there is no
regression for other cases (turns, restrictions).
@mjjbell mjjbell force-pushed the fix-traffic-signal-bug branch from 7c751d9 to ad9ef2c Compare March 16, 2024 19:44
@mjjbell mjjbell merged commit 6e77d53 into Project-OSRM:master Mar 17, 2024
20 checks passed
@GitBenjamin
Copy link
Contributor Author

@GitBenjamin I'm going to add the fix to this PR for problem 1.

It's unclear if problem 2 would also be fixed, but we can address that in a separate PR once we have identified a minimal reproduction test case.

Problem 2 no longer occurs when I call Match API again, after I modify the traffic light compressed logic. I can try to do an analysis of that part of osm data again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants